Skip to content
This repository was archived by the owner on Sep 9, 2021. It is now read-only.

Conversation

@TrySound
Copy link
Contributor

@TrySound TrySound commented Jul 5, 2017

I disabled some rules locally and disabled linebreak-style because it sucks for windows users and auto conversion happen in git out of the box.

Issues

@TrySound
Copy link
Contributor Author

TrySound commented Jul 5, 2017

Any ideas why they are failed?

@TrySound
Copy link
Contributor Author

TrySound commented Jul 5, 2017

Why webpack canary is default?

@joshwiens
Copy link
Member

Latest is default

@joshwiens
Copy link
Member

joshwiens commented Jul 5, 2017

Canary run once in Travis on Node8. If the question is why is it targeting latest that's because it's a pipe dream to think anyone, to include me is going to keep the travis test Webpack version current across all of our libs.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the various config files for correctness, webpack-defaults sometimes leaves old bits in place on the first run, otherwise 👍 . I do the README upgrade during the day 😛

.eslintrc Outdated
@@ -0,0 +1,20 @@
{
"extends": "webpack",
"rules": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this added intentionally or was this the previous config ? webpack-defaults doesn't always remove all previous settings, when applied for the first time, you need to double check here :)

⚠️ This can also happen in the .bablerc file

src/index.js Outdated
outputOptions[name] = this.options.worker.output[name];
});
}
/* eslint-disable no-underscore-dangle */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is an entire rule move it to the top :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entire. Just for this case with child compiler.

test/test.js Outdated
@@ -0,0 +1,221 @@
/* eslint-disable linebreak-style */

import assert from 'assert';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use the buildin Jest Assertions expect instead ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Jest internals + snapshots only.

@michael-ciniawsky michael-ciniawsky changed the title Add webpack-defaults refactor: apply webpack-defaults Jul 5, 2017
@michael-ciniawsky michael-ciniawsky added this to the 1.0.0 milestone Jul 5, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jul 5, 2017

You merged a few PR's, but some don't conform to the Angular Commit Convention, standard-version (webpack-defaults) requires this convention for CHANGELOG autogeneration. Please rebase the history, I normally at least check if the PR commit message conforms if I'm not that useful otherwise 😛 , but you were faster :D I will mark the conflicting commits here

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jul 5, 2017

2d7e1a0 => chore(release): 0.8.1
#80 => test: improve named workers via `options`
#79 => docs(README): add typescript example
#78 => feat: add `options` validation (`schema-utils`)
#66 => docs(README): add worker message example

"strict": 0
}
"test": "jest",
"posttest": "eslint .",
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a leftover aswell ? 🙃

src/index.js Outdated
}
compilation.cache = compilation.cache[subCache];
}
/* eslint-enable no-param-reassign */
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule should be relaxed in eslint-config-webpack, I see this line it in every webpack-defaults PR :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do a ton of it but really shouldn't but none the less, we do need to update the config to back off on it a bit.

"peerDependencies": {
"webpack": ">=0.9 <2 || ^2.1.0-beta || ^2.2.0"
"webpack": "^2.0.0 || >= 3.0.0-rc.0 || ^3.0.0"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the 3.0.0-rc.0. We don't support beta & RC builds once the major in question hits @latest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why defaults added it?

Copy link
Member

@joshwiens joshwiens Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, one sec.

It's already fixed in master. Pick up webpack-defaults@1.5.0

package.json Outdated
"eslint-plugin-import": "^2.2.0",
"jest": "^20.0.4",
"lint-staged": "^4.0.0",
"mocha": "^3.2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove mocha

.gitignore Outdated
.idea
.vscode
*.sublime-project
*.sublime-workspace
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? Is this stuff necessary? Should be always developer responsibility, not project.

@@ -0,0 +1,20 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d3viant0ne Do you know .eslintrc without extension is deprecated?
http://eslint.org/docs/user-guide/configuring#configuration-file-formats

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's on the list to be changed when we switch everything over to just use Prettier in defaults 2.0

@renchap renchap mentioned this pull request Jul 13, 2017
@renchap
Copy link
Contributor

renchap commented Jul 17, 2017

Any news on this PR? This is blocking our update to Webpack 3.

@mikegreiling
Copy link

Is there anything we can do to help this PR along? We're unable to upgrade to webpack v3 until this is merged 🙁

@dbslone
Copy link

dbslone commented Aug 1, 2017

@TrySound @d3viant0ne - Any updates with this PR? I can pick up where the PR left off to get the tests passing on Travis but wanted to check in first so i don't duplicate anything that has already been done.

@TrySound
Copy link
Contributor Author

TrySound commented Aug 1, 2017

@dbslone No updates yet. You may finish it.

@dbslone
Copy link

dbslone commented Aug 8, 2017

The tests are passing locally for me. At first I thought it was because I was on node 8 but after installing node 6.11 the tests are still passing.

Is anything else left for this PR to be approved?

@joshwiens
Copy link
Member

joshwiens commented Aug 8, 2017

@dbslone - I'm going to get a patch out with #85 first & wrap this up after the fact.

@joshwiens joshwiens self-assigned this Aug 12, 2017
@michael-ciniawsky
Copy link
Member

Got it stats.toJson('minimal') => stats.toJson() 🎉 😛

@codecov
Copy link

codecov bot commented Oct 12, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@2051a3a). Click here to learn what that means.
The diff coverage is 77.41%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #81   +/-   ##
=========================================
  Coverage          ?   64.86%           
=========================================
  Files             ?        5           
  Lines             ?       74           
  Branches          ?       26           
=========================================
  Hits              ?       48           
  Misses            ?       23           
  Partials          ?        3
Impacted Files Coverage Δ
src/workers/InlineWorker.js 0% <0%> (ø)
src/cjs.js 0% <0%> (ø)
src/workers/index.js 100% <100%> (ø)
src/Error.js 100% <100%> (ø)
src/index.js 86.11% <86.11%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2051a3a...a6d8208. Read the comment docs.

@michael-ciniawsky michael-ciniawsky force-pushed the webpack-defaults branch 3 times, most recently from e21a940 to 8c5dd95 Compare October 19, 2017 13:14
@joshwiens joshwiens merged commit 09705fb into master Oct 21, 2017
@michael-ciniawsky michael-ciniawsky removed this from the 2.0.0 milestone Oct 24, 2017
@michael-ciniawsky michael-ciniawsky deleted the webpack-defaults branch February 25, 2018 06:37
TheLD6978 pushed a commit to TheLD6978/worker-loader that referenced this pull request Apr 16, 2021
* Add webpack-defaults

* ci(Appveyor): Run on latest

 - Currently a change that is in defaults master

* chore(defaults): v1.4.0...1.6.0

* fix(cjs): export pitching loader

* test: stats.toJson('minimal') => stats.toJson()

* style: improve code style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants